Repository navigation
census: the call row carries the demanded set, so served != lane means what it says again (#969) - #1796
census: the call row carries the demanded set, so served != lane means what it says again (#969)#17967vignesh wants to merge 9 commits into
Conversation
…s what it says again (Agent-Field#969) Since the chooser demands the whole admitted set (provider.only with fallbacks off), the router may serve any member of it, so Record.Lane (the ranked head) stopped being the only machine the request would accept. The census read served != lane as the router going somewhere we did not choose, which after the set-demand is often the router choosing a different member of the set we did choose. Add Record.Lanes []string, filled from laneChoice.Only when the demand is still being sent, carrying the whole set when it has two or more members (a one-machine demand is left to Lane alone). The census counts asked != served only when served fell OUTSIDE the admitted set. callrows.Row embeds calllog.Record, so the census reads the new field the day it lands.
ZeroPoint95
left a comment
ZeroPoint95
left a comment
There was a problem hiding this comment.
Hi @7vignesh, thanks for this fix!
I’m requesting changes because demandedLanes records the original choice rather than the demand actually sent for each attempt:
-
Rescue requests retain the original set. A rescue sends
only: [B], but its row recordslanes: [A, B]. If A serves that request, the census incorrectly treats it as agreement. -
Narrowed retries retain refused machines. After
cinnabarrefuses, the retry sendsonly: [tinder], but its row still recordslanes: [cinnabar, tinder]. It also retainsLane: cinnabar, so simply omittingLanesfor the singleton would leave incorrect attribution.
Please capture the final composed demand for each attempt and preserve it for both start and finish rows. For singleton demands, either retain the singleton in Lanes or make Lane name the actual demanded machine. Please add wire-versus-log regression tests for both cases.
CI and the full local battery passed. The two provider reproductions and the census violation fixture fail at this head and pass on the base.
…iginal choice ZeroPoint95's review on Agent-Field#1796 caught that demandedLanes read the original laneChoice.Only, so a rescue (sends only:[B]) and a narrowed retry (drops a refused machine) recorded the whole original set and misattributed Lane. Capture provider.only as wirePreferences composed it, at encode time, onto the trace; carry it onto the openAttempt so a finish row written after a later attempt re-encoded still describes its own. demandedLanes now reads that composed set, and recordedLane names the single machine a narrowed attempt really demanded rather than the ranked head it recovered from. Adds the two wire-versus-log regression tests the review asked for: a rescue arm records only its one machine, and a narrowed retry does not keep the set it no longer demands and attributes to the machine it does.
|
Thanks @ZeroPoint95, you were right on both counts -
Added the two wire-versus-log regression tests you asked for:
Both fail at the previous head and pass now; the original set and emptiness tests still pass, and the census fixture is unchanged (it already counts agreement as One caveat unchanged from before: I verified on a Windows box, so the two pre-existing failures I noted (the tree-walking canary over |
santoshkumarradha
added
bug
ZeroPoint95
left a comment
ZeroPoint95
left a comment
There was a problem hiding this comment.
@7vignesh, the rescue and narrowed-retry changes now pass the independent checks.
Both now pass independent wire-versus-log checks for their start and finish rows. One remaining issue needs fixing:
-
Preserve the snapshot through automatic closing.
closeOpenAttemptclearstrace.openbefore callingrecord, sobodyDemandedOnlyfalls back to the latest encode’s demand rather thanopen.demandedOnly.A deterministic reproduction using real encodes opens attempt 1 with
only: [cinnabar, tinder], then encodes attempt 2 withonly: [tinder]. Attempt 2’s start automatically closes attempt 1 ashopped. For the same attempt-1 ID, the start recordslane: cinnabar, lanes: [cinnabar, tinder], but its ending recordslane: tinderwith no set.Please preserve the captured snapshot until the closing row is built, and add coverage to the existing unconditional-close regression. This reproduction fails at the new head and passes at the previous head. Ordinary 429 retries explicitly write their endings and pass; this finding concerns the fallback closing mechanism.
-
Documentation consistency — nonblocking.
Please remove the obsolete duplicateddemandedLanescommentary about readinglaneChoice.Only. Also correct the new “[Lane] first” promise: a three-to-two retry can retain the originalLanewhileLanescorrectly names the remaining pair. The census membership calculation remains correct.
CI is green. Local checks completed; the provider suite had an allocation-law failure also reproduced on dev and a cancellation-test race that passed its focused retry.
… fix docs ZeroPoint95's second review caught that closeOpenAttempt clears trace.open before recording, so a finish row built on the fallback close read the latest encode's demand rather than the closed attempt's. Carry the attempt's snapshot through recordFacts.demandedOnly and read it off the ended row in bodyDemandedOnly, keyed on ended (set only on that row) so an attempt closed carrying no demand is not handed a later one. Also remove the stale duplicated demandedLanes doc that still described reading laneChoice.Only, and correct the Record.Lanes doc: Lane is a member of the set but not necessarily its first element, since a three-to-two retry can keep the original ranked head as Lane while Lanes names the surviving pair. Adds TestAHoppedAttemptKeepsItsOwnDemandedSetThroughTheClose, which fails without the carry and passes with it.
|
Thanks @ZeroPoint95, both addressed. 1. Preserve the snapshot through the automatic close. You were right:
Added 2. Documentation. Removed the stale duplicated On the environment: I verified on Windows, where |
3 participants
What changed
Since #937 the chooser demands the SET it admitted -
provider.onlywithallow_fallbacks: falseover every machine that survived the gate, the prune, the ceiling and the quality floor - and the router must stay inside that set while being free to pick which member serves. But the call log'sRecord.Laneis one machine (the ranked head), so a row whereserved != lanecan no longer tell "the router chose a different member of the set we admitted" from "the router went somewhere we never named". The census family built on it (asked ≠ served) quietly changed what it measures (#969).internal/calllog: addRecord.Lanes []string(json:"lanes,omitempty"), the whole demanded set.callrows.Rowembedscalllog.Record, so the census reads it the day it lands.internal/provider/calllog.go:demandedLanes(knobs)fillsrecord.Lanesfromknobs.laneChoice.Onlywhererecord.Laneis filled, under the same discipline assoleDemandedLane(only while the demand is still being sent, never off knobs the widen relaxed). Empty when the request demanded nothing or demanded exactly one - the single caseLanealone already answers, and the state of every pre-Lanes: the chooser demands the set it admitted, instead of suggesting it #937 row.cmd/codeaf-census/report.go:servedWasAdmitted- a row with a set agrees when served is a MEMBER of it; a row without one agrees when served matches the single ranked name. Only served landing outside counts as asked ≠ served.Scoped to the first half of the issue. The
cmd/*-replay/Policy.Demandwidening the issue names as the second half is deliberately left out - it is only worth doing once the log can say whether the served machine was inside a set, which this change is.How it was checked
go test ./internal/provider/ -run 'TestADemandedSetLeavesEveryAdmittedMachineOnTheRow|TestASingleMachineDemandLeavesNoSetOnTheRow'- end-to-end throughpacedPair(two believed-in machines behind one model, from the issue's named rig): one real demanded call writes a row carrying both admitted machines inlanes, the ranked head inlane, and served inside the set. Single-machine and no-demand calls write no set.go test ./cmd/codeaf-census/ -run TestServedInsideTheDemandedSetIsNotAskedNotServed- a log with three rows (served inside the set, served outside a lone name, served outside the set) reports asked ≠ served = 2 of 3, counting the in-set row as agreement.go test ./internal/calllog/ -short,go test ./cmd/codeaf-census/- green.go vetandgofmtclean on all touched files;go build ./internal/calllog/ ./internal/provider/ ./cmd/codeaf-census/succeeds.Note: the full
internal/calllogsuite and the fullinternal/providersuite each have one failure on this Windows box that reproduces on cleandevwith my changes stashed -TestNoTestInTheTreeWritesIntoTheLedgerOfWhoeverRanIt(a canary that runs other packages as children, several of which don't build on Windows:internal/sessionis//go:build !windows, others hit SQLiteout of memory) andTestAFiredHedgeAndARefusedHedgeAreBothDecisions(aTempDircleanup race on a trace file still held open). Both are pre-existing and unrelated to this change.Checklist
docs/changes/unreleased/1796-demanded-lanes-on-the-row.md.docs/design/recovery/census-20260910.mdis a captured record and left as written..github/known-red.txt.Closes #969.